-
Notifications
You must be signed in to change notification settings - Fork 381
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add ECDSA signature verification #372
Conversation
@yelhousni can you do a review of that one before merge? |
Merged develop and fixed conflicts. It is good to merge from my side. @yelhousni said that will post optimisations in separate PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! 👍
We can merge this PR and track optimizations in separate PRs.
@yelhousni should we merge now or wait for replacing the external dependency for out-of-circuit ecdsa with latest |
To do so we have to wait for Consensys/gnark-crypto#310 to be merged I guess. So either we wait or we merge this and follow with a quick PR for the dependency. |
Initial PR for discussion. If the API looks OK, will add documentation and merge for now.
This PR implements also emulated short-Weierstrass package, which for now assumes the special case for secp256k1 (a4=0), but in general can be generalised to any a4. I didn't want to do it now because then we would need to have more parameters etc. and I didn't want to eagerly generalise.
A few things to do before merge:
frontend.Variable
toemulated.Element[T]
. I think long-term would be better to have something which returns actual types not empty interface.This implementation right now is very heavy and requires 3.7M constraints without range checks, but I think it is a good base for optimisation. There are several optimisation options:
DRAFT: should be merged after #395.